-
Notifications
You must be signed in to change notification settings - Fork 18.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tools: uploader.py: verify correct install #29164
Conversation
rafasaurus
commented
Jan 28, 2025
- Implement functionality to verify correct firmware installation
- Utilize existing code with an additional argument for verification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"verify" is really, "verify that the flash worked".
You're re-using the functionality, but that terminology isn't really applicable in this case.
I'm OK with the current name of the parameter, but if we can find something better that would be good!
--assert-firmware-is
would work, but a little verbose
In any case, a slightly better description is called for...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... also, please fold all of this into the one commit . The one starting with Tools:
looks appropriate.
Should we just squash and merge? |
feccd9f
to
c601402
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid merge commits in your history - this needs a rebase again.
I've also requested a change you suggested yourself. I think the suggestion makes it unambiguous what the new feature does?
c0682e2
to
43da991
Compare
Changed, rebased and tested on the board. |
e37fc96
to
47d20dd
Compare
- Implement functionality to verify correct firmware installation - Utilize existing code with an additional argument for verification
47d20dd
to
7ef59ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The permissions changed on the modified file from 0755 to 0644. I've fixed this.
I've tested this locally and it works as expected.
Marking as MergeOnCIPass - thanks!